fs: make `open` and `close` stream override optional when unused #40013

aduh95 · 2021-09-06T09:38:11Z

When using createReadStream or createWriteStream with a specific
file descriptor or FileHandle instead of a path, open method is not
used, there is no point in forcing users to provide it.
When using createReadStream or createWriteStream with autoClose
set to false, close method is not used, there is no point in forcing users
to provide it.

lib/internal/fs/streams.js

doc/api/fs.md

jasnell · 2021-09-06T14:32:24Z

lib/internal/fs/streams.js

+    // When fd is a FileHandle we can listen for 'close' events
+    if (options.fs) {
+      // FileHandle is not supported with custom fs operations
+      throw new ERR_METHOD_NOT_IMPLEMENTED('FileHandle with fs');


The error message here could be more descriptive (could just use the same message as the comment)

The error message currently is The FileHandle with fs method is not implemented. ERR_METHOD_NOT_IMPLEMENTED may not be the fittest here, but I'm afraid changing it would be a breaking change, best left to another PR. Or do you have a suggestion that would work with ERR_METHOD_NOT_IMPLEMENTED?

doc/api/fs.md

nodejs-github-bot · 2021-09-12T21:43:30Z

CI: https://ci.nodejs.org/job/node-test-pull-request/39929/

nodejs-github-bot · 2021-09-12T22:47:33Z

CI: https://ci.nodejs.org/job/node-test-pull-request/39931/

github-actions · 2021-09-16T22:35:24Z

Commit Queue failed

- Loading data for nodejs/node/pull/40013
✔  Done loading data for nodejs/node/pull/40013
----------------------------------- PR info ------------------------------------
Title      fs: make `open` and `close` stream override optional when unused (#40013)
   ⚠  Could not retrieve the email or name of the PR author's from user's GitHub profile!
Branch     aduh95:fs-streams-optional-open-close -> nodejs:master
Labels     fs, semver-minor, author ready, needs-ci
Commits    7
 - fs: make `open` and `close` stream override optional when unused
 - fixup! fs: make open and close stream override optional when unused
 - fixup! fs: make `open` and `close` stream override optional when unused
 - fixup! fs: make `open` and `close` stream override optional when unused
 - fixup! fs: make `open` and `close` stream override optional when unused
 - fixup! fs: make `open` and `close` stream override optional when unused
 - fixup! fs: make open and close stream override optional when unused
Committers 2
 - Antoine du Hamel 
 - GitHub 
PR-URL: https://github.com/nodejs/node/pull/40013
Reviewed-By: James M Snell 
------------------------------ Generated metadata ------------------------------
PR-URL: https://github.com/nodejs/node/pull/40013
Reviewed-By: James M Snell 
--------------------------------------------------------------------------------
   ⚠  Commits were pushed since the last review:
   ⚠  - fixup! fs: make open and close stream override optional when unused
   ℹ  This PR was created on Mon, 06 Sep 2021 09:38:11 GMT
   ✔  Approvals: 1
   ✔  - James M Snell (@jasnell) (TSC): https://github.com/nodejs/node/pull/40013#pullrequestreview-747279667
   ✔  Last GitHub Actions successful
   ℹ  Last Full PR CI on 2021-09-12T22:47:33Z: https://ci.nodejs.org/job/node-test-pull-request/39931/
- Querying data for job/node-test-pull-request/39931/
✔  Build data downloaded
   ✔  Last Jenkins CI successful
--------------------------------------------------------------------------------
   ✔  Aborted `git node land` session in /home/runner/work/node/node/.ncu

https://github.com/nodejs/node/actions/runs/1243428716

When using `createReadStream` or `createWriteStream` with a specific file descriptor or `FileHandle` instead of a path, `open` method is not used, there is no point in forcing users to provide it. When using `createReadStream` or `createWriteStream` with `autoClose` set to false, `close` method is not used, there is no point in forcing users to provide it. PR-URL: nodejs#40013 Reviewed-By: James M Snell <jasnell@gmail.com>

aduh95 · 2021-09-16T22:47:43Z

Landed in 8a92018

When using `createReadStream` or `createWriteStream` with a specific file descriptor or `FileHandle` instead of a path, `open` method is not used, there is no point in forcing users to provide it. When using `createReadStream` or `createWriteStream` with `autoClose` set to false, `close` method is not used, there is no point in forcing users to provide it. PR-URL: #40013 Reviewed-By: James M Snell <jasnell@gmail.com>

Notable changes: crypto: * (SEMVER-MINOR) add rsa-pss keygen parameters (Filip Skokan) #39927 doc: * add Ayase-252 to collaborators (Qingyu Deng) #40078 fs: * (SEMVER-MINOR) make `open` and `close` stream override optional when unused (Antoine du Hamel) #40013 http: * (SEMVER-MINOR) limit requests per connection (Artur K) #40082 src: * (SEMVER-MINOR) add --no-global-search-paths cli option (Cheng Zhao) #39754 * (SEMVER-MINOR) add option to disable global search paths (Cheng Zhao) #39754 * (SEMVER-MINOR) make napi_create_reference accept symbol (JckXia) #39926 stream: * (SEMVER-MINOR) add signal support to pipeline generators (Robert Nagy) #39067 PR-URL: TODO

When using `createReadStream` or `createWriteStream` with a specific file descriptor or `FileHandle` instead of a path, `open` method is not used, there is no point in forcing users to provide it. When using `createReadStream` or `createWriteStream` with `autoClose` set to false, `close` method is not used, there is no point in forcing users to provide it. PR-URL: #40013 Reviewed-By: James M Snell <jasnell@gmail.com>

Notable changes: crypto: * (SEMVER-MINOR) add rsa-pss keygen parameters (Filip Skokan) #39927 doc: * add Ayase-252 to collaborators (Qingyu Deng) #40078 fs: * (SEMVER-MINOR) make `open` and `close` stream override optional when unused (Antoine du Hamel) #40013 http: * (SEMVER-MINOR) limit requests per connection (Artur K) #40082 src: * (SEMVER-MINOR) add --no-global-search-paths cli option (Cheng Zhao) #39754 * (SEMVER-MINOR) add option to disable global search paths (Cheng Zhao) #39754 * (SEMVER-MINOR) make napi_create_reference accept symbol (JckXia) #39926 stream: * (SEMVER-MINOR) add signal support to pipeline generators (Robert Nagy) #39067 PR-URL: TODO

Notable changes: crypto: * (SEMVER-MINOR) add rsa-pss keygen parameters (Filip Skokan) #39927 doc: * add Ayase-252 to collaborators (Qingyu Deng) #40078 fs: * (SEMVER-MINOR) make `open` and `close` stream override optional when unused (Antoine du Hamel) #40013 http: * (SEMVER-MINOR) limit requests per connection (Artur K) #40082 src: * (SEMVER-MINOR) add --no-global-search-paths cli option (Cheng Zhao) #39754 * (SEMVER-MINOR) add option to disable global search paths (Cheng Zhao) #39754 * (SEMVER-MINOR) make napi_create_reference accept symbol (JckXia) #39926 stream: * (SEMVER-MINOR) add signal support to pipeline generators (Robert Nagy) #39067 PR-URL: #40175

nodejs#40013 changed the behavior of `readStream.path`, it'll be `undefined` if `fd` is specified.

it'll be `undefined` if `fd` is specified. Refs: #40013 PR-URL: #40252 Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

it'll be `undefined` if `fd` is specified. Refs: nodejs#40013 PR-URL: nodejs#40252 Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

nodejs-github-bot added fs Issues and PRs related to the fs subsystem / file system. needs-ci PRs that need a full CI run. labels Sep 6, 2021

aduh95 force-pushed the fs-streams-optional-open-close branch from c3a32ad to f1022f7 Compare September 6, 2021 09:39

aduh95 added the semver-minor PRs that contain new features and should be released in the next minor version. label Sep 6, 2021

aduh95 force-pushed the fs-streams-optional-open-close branch 2 times, most recently from fbab685 to fe5f720 Compare September 6, 2021 10:54